fix: load full history when compacting a limited session#3827
fix: load full history when compacting a limited session#3827winklemad wants to merge 9 commits into
Conversation
OpenAIResponsesCompactionSession._ensure_compaction_candidates loaded the underlying history with a bare underlying_session.get_items(), which resolves to session_settings.limit and returns only the latest N items. In input/auto mode those truncated items are what gets sent to responses.compact, and run_compaction then clears the session and replaces it with the compacted summary — so any history older than the limit window is silently and permanently lost. The restore/replace paths already read the full history through _get_all_underlying_session_items() (limit=_ALL_SESSION_ITEMS_LIMIT), added in openai#3117; the candidate-loading path was missed. Use the same helper there so compaction always operates on the complete stored history.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
seratch
left a comment
There was a problem hiding this comment.
Using _get_all_underlying_session_items() is correct for explicit input compaction. However, the current change also loads full history for thresholding while ordinary auto with a stored response_id still resolves to previous_response_id. That request does not include the full local history, but the session is then cleared and replaced, so older items can still be silently dropped; this patch can also make that compaction trigger earlier.
Please make auto fall back to full-history input compaction whenever the stored history is not fully represented by the limited session view, while leaving explicitly requested previous_response_id behavior opt-in. Add a no-force regression test covering that auto path before replacement.
…imited In auto mode with a stored response_id, compaction resolves to previous_response_id, which does not send the local history. When the underlying session's default view is limit-bounded and does not cover the full stored history, clearing and replacing the session with the server-derived summary can still drop the unrepresented items. Auto now falls back to full-history input compaction whenever the limited view does not cover the stored history; an explicitly requested previous_response_id compaction stays opt-in. Add a no-force regression test for the auto path, and guard the test assertions against an Optional input value for pyright.
|
Thanks @seratch — you're right, the
Full |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5469581c1c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
When auto compaction falls back to input because the limited session view did not cover the full history, the stored response_id still points at a server response that omits the hidden items. Once the session later shrinks back within the limit, a forced compaction would otherwise switch back to previous_response_id and replace the full-history summary with a server-derived one that drops those items. Mark the response_id unsafe on fallback, mirroring the store=False path, and cover it with a regression test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10471fd7f5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The auto->input fallback checked the underlying session's default view, which misses a limit supplied per run (e.g. RunConfig session settings) on an otherwise-unlimited store: the response only saw the limited window, but the check reported full coverage and kept using previous_response_id. Remember the limit the Runner uses to prepare each turn's input via get_items and compare against that window instead, so the fallback triggers whenever the response did not see the full history.
|
@seratch ready for another look when you get a chance — the |
seratch
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous-response fallback, unsafe ID reuse, and per-run limits. The remaining blocker is ownership of the per-run limit.
_last_prepared_input_limit is one mutable field updated by every public get_items() call, rather than state tied to a specific run or response. If A prepares with limit=3, B prepares with limit=None, and A then completes, A can read B's limit and use previous_response_id even though response A omitted older history. A fresh compaction wrapper on RunState resume has the same problem because resume skips session input preparation.
Please carry the effective history-window metadata with the specific run/response into both compaction and deferred compaction instead of inferring it from shared get_items() side effects. Please also add controlled-overlap and fresh-wrapper-resume regressions that assert the surviving operation and full stored history.
|
Thanks @seratch — good catch on the ownership issue. Reworked it so the history window travels with the specific response instead of being inferred from shared
Added the two regressions you asked for, both asserting the surviving operation compacts from the full stored history:
Both fail on the previous commit and pass now. Full |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d79002827f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # itself, so compaction can tell whether it saw the full stored history instead of | ||
| # inferring it from shared session state (which breaks under interleaved runs and | ||
| # fresh wrappers on resume that skip input preparation). | ||
| input_history_limit = resolve_session_history_limit(session, session_settings) |
There was a problem hiding this comment.
Thread run limits through save_turn_items_if_needed
This only preserves the per-run limit for callers that pass session_settings into save_result_to_session; the normal final-output and run-again paths still go through save_turn_items_if_needed, whose signature has no session_settings parameter and calls this function without one. With an OpenAIResponsesCompactionSession wrapping an otherwise-unlimited store and RunConfig(session_settings=SessionSettings(limit=N)), those paths resolve input_history_limit to None here even though prepare_input_with_session sent only the limited window, so auto compaction can still use previous_response_id and replace the full local history with a server summary that never saw the older items.
Useful? React with 👍 / 👎.
|
Follow-up: I widened the fix so the per-run window now flows through every save path that can reach compaction, not just the direct Added regressions for each: the final-output path ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2ace6c9ec
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| prepared_view = await self.underlying_session.get_items(limit=input_history_limit) | ||
| full_view = await self._get_all_underlying_session_items() | ||
| return len(prepared_view) >= len(full_view) |
There was a problem hiding this comment.
Track pre-save truncation instead of post-save length
When a limited run starts with history that fits within the limit but the current turn's saved input/output pushes the stored session over that limit, this recomputes prepared_view after save_result_to_session() has already added the new items. In that case the response actually saw all pre-existing history, so previous_response_id is safe, but this returns false and forces full-history input compaction; for large sessions at the limit this can turn a safe server-side compaction into an oversized/expensive input request. Carry whether the model input was actually truncated at preparation time, or compare against the pre-save history, rather than the post-save session length.
Useful? React with 👍 / 👎.
|
One more refinement on the efficiency edge: a limited run whose pre-existing history already fit within the window shouldn't be forced onto full-history Coverage is now resolved against the pre-save store (the state the response was actually prepared from) in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2788e93e1c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| history_limit = resolve_session_history_limit(session, session_settings) | ||
| if history_limit is None: | ||
| input_covered_full_history = True |
There was a problem hiding this comment.
Honor wrapped session limits before trusting response IDs
When session is an OpenAIResponsesCompactionSession wrapping a limited store, resolve_session_history_limit() sees the wrapper's session_settings as None, even though prepare_input_with_session() later delegates get_items(None) to the underlying session and that underlying default limit truncates the model input. This branch then records input_covered_full_history=True, so normal Runner saves with an underlying SessionSettings(limit=N) keep using previous_response_id and can replace the full local store with a server summary that never saw the older hidden items. Propagate the wrapped session's effective limit here, or leave the coverage unknown so run_compaction() can compare against the full underlying history.
Useful? React with 👍 / 👎.
|
This round is a correctness fix, not just efficiency. When the compaction session wraps a store that carries its own It now walks the delegate chain ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5570d8091a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Fewer stored items than the window means the window held them all; a full | ||
| # window may still hide older items, so treat that as truncated. | ||
| prepared_view = await session.get_items(limit=history_limit) | ||
| input_covered_full_history = len(prepared_view) < history_limit |
There was a problem hiding this comment.
Treat exact-limit histories as fully covered
The new pre-save check still classifies an exact-fill window as truncated: when the stored history has exactly history_limit items before the turn, the model input contained the full history, but len(prepared_view) < history_limit is false. If the current turn then pushes the session over the limit and compaction triggers, auto mode falls back to full-history input even though previous_response_id is safe, which can turn a server-side compaction into an unnecessarily large or failing input compaction request. Consider checking whether there is an item beyond the limit, e.g. by comparing against a pre-save full count or requesting history_limit + 1.
Useful? React with 👍 / 👎.
|
Tightened the pre-save coverage check to be exact. It now reads one item beyond the window ( |
|
@seratch this is ready for another look — consolidating where it landed, since the fixes ended up spread across a few commits. The ownership blocker is resolved. The history-window state no longer lives in a shared mutable field ( Both regressions you asked for are in:
Also hardened along the way: per-run Green on the current head: the compaction suite (59 tests) plus ruff, mypy, and pyright clean on the changed files. |
What & why
OpenAIResponsesCompactionSession._ensure_compaction_candidatesloads the underlying history with a bareunderlying_session.get_items(). For a session configured with a limit (SessionSettings(limit=N)),get_items()resolves the limit tosession_settings.limitand returns only the latest N items.In
input/autocompaction mode those truncated items are exactly what gets sent toresponses.compact, andrun_compactionthen clears the underlying session and replaces it with the compacted summary. So every item older than the limit window is silently and permanently lost. Compaction runs automatically once the candidate threshold is reached (noforceneeded), so this bites any user who pairs a limited session with compaction.Minimal repro:
Fix
The restore/replace paths already read the complete history through
_get_all_underlying_session_items()(limit=_ALL_SESSION_ITEMS_LIMIT), introduced in #3117; the candidate-loading path was missed. Use the same helper so compaction always operates on the full stored history.Tests
Added
test_run_compaction_uses_full_history_when_underlying_session_has_limit: a realSQLiteSession(limit=3)holding 8 items must send all 8 toresponses.compact(fails before the fix — only the latest 3 arrive). Fulltests/memory/suite (146 tests) passes; ruff and mypy clean.Found by reading the code; no linked issue.